Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Getting started documentation #127

Merged
merged 1 commit into from
Jan 18, 2025

Conversation

danielskinstad
Copy link
Collaborator

@danielskinstad danielskinstad commented Jan 8, 2025

No description provided.

Copy link
Collaborator

@vpodzime vpodzime left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks generally good to me. I didn't add comments for small things, leaving those for the final-ish review(s)

README.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@lluiscampos lluiscampos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I run out of time, I'll continue later!)

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@lluiscampos lluiscampos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done now with the review. Overall looks pretty comprehensive to me. You are covering everything that I listed in the JIRA task so from that side is good.

I added few more comments.

I suggest you first do now a round of small fixing (based on my comments) and then besides us the developers ping also our PO an docs owner for final review. We might get more feedback wrt the technical level of this guide and we might need to either write more stuff and/or create follow-up tasks to refactor some parts (example: I could see the PO suggesting to move the HM certs to mender-mcu)

Thank you for tackling this one 🍿

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@danielskinstad danielskinstad changed the title WIP: getting started documentation Getting started documentation Jan 10, 2025
Copy link

@estenberg estenberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a few suggestions.

README.md Outdated Show resolved Hide resolved
```
cmake -C cmake/CMake_posix_defaults.txt -B build tests/smoke
```
The `mender-mcu` client can take up to two certificates; typically one for the Server API calls, and

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good you document this clearly but this seems like a limit we need to increase. If certificates need rotation we need to have the building blocks. I think we at least need support for 4 (two for each).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This brings a point that we should rather move all the certs handling into mender-mcu core repo. Right now in the core we define if there will be one or two, but then is the application (mender-mcu-integration or any other user application) that will add the certs via tls_credential_add(...). Instead, we can (and should?) have the mender client init function add the certs.

I suggest we create a task for this and do not modify the README further. In the task we shall write that the README needs to be updated accordingly.

Copy link
Collaborator

@vpodzime vpodzime Jan 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't quite see how this is part of the mender-mcu (client) scope. In my eyes, this is part of the "environment", just like the network setup (including TLS support) and other similar things.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is a valid point (and the original point for having it out of Mender hands).

But it gets tricky for us when down in the code zsock_setsockopt needs to know the CA tags (up to 4).

At least we should think about how to simplify this part 🤔 We could maybe have yet another callback to get this TLS_SEC_TAG_LIST from the user.

To move the topic forward: discuss in next client team meeting and create a task either for moving it to mender-mcu (most likely not) or other ideas for decoupling it a bit (like mine above or others).

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@vpodzime vpodzime left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

Ticket: MEN-7871

Signed-off-by: Daniel Skinstad Drabitzius <[email protected]>
@danielskinstad danielskinstad merged commit 98c0509 into mendersoftware:main Jan 18, 2025
2 checks passed
@danielskinstad danielskinstad deleted the get-started-docs branch January 18, 2025 16:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants